Repository navigation
docs/lora: a Trap-1 row for main's current pin (ed43da0b4), and the rule that the HRX drift is transient - #1399
Conversation
…is transient Trap 1's table had rows for the c20 pin and for 4a375bf2d, and main now pins ed43da0b4 — on a page whose whole point is that the zero-adapter offset is a property of a pin. Measured on a clean build of that pin (source tree at ed43da0b4 with an empty `git status`, frozen before use, device named in the log): pin base zero-adapter offset a5271d9 2.265140 2.272791 +0.34 % 4a375bf2d 2.267098 2.265357 -0.08 % ed43da0b4 2.265140 2.272791 +0.34 % <- same as a5271d9 So the 4a375bf2d drift is gone rather than compounded: all four metrics (base, zero-adapter, after-drop and the +/-eps probe, plus the x10 response) are back to a5271d9's exact values, and the row was taken twice per pair in one ticket (reference pair and this pin alternating) so it is a measured identity, not a lookalike. Consequence stated on the page: a loss from one pin ago is neither an upper nor a lower bound on today's — re-measure the row, do not interpolate. Two smaller corrections from the same round: - the cost table named engine#973 but not the pin its numbers came from; it now says llama.cpp 32559003a (the pin main carried then), so it cannot be read as a claim about the current pin; - a new paragraph, "attribute the build before you record a row — the commit is not enough": two source trees at the same sha are not the same build (one is the pin, one is that sha plus a lane's four modified files), so a row records pin sha + working-tree cleanliness + the frozen libraries' sha256sum + the device line. Refs #973, #1357 (the page's own merge).
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at d4bf65c9.
The row is right and so is the arithmetic, checked against the table rather than the prose:
a5271d9 (c20) base 2.265140 zero 2.272791 +0.34 %
4a375bf2d (engine #1120) base 2.267098 zero 2.265357 -0.08 %
ed43da0b4 (current) base 2.265140 zero 2.272791 +0.34 % <- byte-identical to a5271d9, as claimed
2.272791 / 2.265140 = 1.003378 -> +0.3378 % ("+0.34 %" ok)
2.265357 / 2.267098 = 0.999232 -> -0.0768 % ("-0.08 %" ok)
And the attributions hold: engine #1120 is "Bump HRX: llama.cpp 4a375bf2d36f + hrx-system 82804197bcbe" — exactly the
4a375bf2d row's label — and ed43da0b4 is origin/main's pin right now, so "current" is true at this head and not a number
that went stale while the PR sat open. Keeping the 4a375bf2d row is the right choice: without it the "transient rather than
monotonic" paragraph has nothing to point at.
Two count/precision notes, both small and both in the spirit of the section:
- "all four metrics back to
a5271d9's exact values" is asserted, not shown. The table makes base and zero visible for
a5271d9, and the Vulkan row covers that metric for all three — buta5271d9's ±ε and ×10 appear nowhere in the page, while
the next paragraph gives this pin's (2.086712/2.256433,13.670712/13.830040). So a reader cannot check the reversion of those
two; they have to trust it, in a section whose whole lesson is do not trust a remembered number. One line —a5271d9's ±ε and ×10
beside the current pin's — turns the claim into something a reader can verify, and it is the same evidence you already have in the
ticket. - "four identical fingerprints" is followed by five (base, zero, drop, ±ε, ×10) — plus Vulkan makes six. Either the sentence says
five, or the list drops one; as written the count reads as the total and it is not.
Neither blocks: the row is correct, the reversion is the correct conclusion for the two metrics a reader can see, and the new
attribution paragraph is the most useful thing in the diff — "the commit is not enough: same sha, two trees, one of them is not the pin"
is precisely the failure mode this table has already had (#1380's backwards pin, which the paragraph would have caught at the door).
…ting them Both count/precision notes from the review of #1399, taken in the section that asks readers not to trust a remembered number: 1. "all four metrics back to a5271d9's exact values" was asserted, not shown — a5271d9's ±ε and ×10 appeared nowhere on the page while this pin's did. The evidence is now a two-row table: the reference pair and this pin side by side on base, zero-adapter, after-drop, ±ε, ×10 and Vulkan, every cell equal and every cell taken twice. That turns the reversion claim into something a reader can check in the table rather than trust from the prose. 2. "four identical fingerprints" was followed by five items (six with Vulkan). The sentence now says six and the table lists exactly six, so the count and the list agree. Refs #973, #1357, #1399.
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 16e54b5d.
Both notes are taken, and the table is the right answer to the first one — the identity is now checkable instead of asserted:
| | base | zero | after drop | ±ε probe | ×10 probe | Vulkan |
| a5271d9, reference pair (runs 1,2)| 2.265140| 2.272791| 2.265140 | 2.086712 / 2.256433 | 13.670712 / 13.830040 | 2.219879|
| ed43da0b4, this row (runs 1,2) | 2.265140| 2.272791| 2.265140 | 2.086712 / 2.256433 | 13.670712 / 13.830040 | 2.219879|
Six columns, every cell equal, two runs per pair — and the base/zero/Vulkan cells agree with the Trap-1 table above it
(2.265140 / 2.272791 / 2.219879), so the two tables cannot drift apart without one of them looking wrong. "All six numbers
equal in all four runs" also matches the row labels, which is the kind of internal consistency this page needs.
One leftover from my second note, because it was fixed in one place and not the other: :98 still reads "ed43da0b4 puts all
four metrics back to a5271d9's exact values", while :103 now says "all six numbers". Same claim, same identity, two different
counts on one page — and the table says six is the right one. Change that one word (or say "the metrics below") and the section is
self-consistent from the drift paragraph through the table.
Everything else stands from my last review: the row's arithmetic (+0.3378 % → "+0.34 %", -0.0768 % → "-0.08 %"), the engine#1120
attribution for 4a375bf2d, ed43da0b4 being origin/main's pin at this head, and the attribution paragraph — which is still the most
useful thing in the diff.
PR Reviewer Guide 🔍(Review updated until commit fd98fc5)Here are some key observations to aid the review process:
|
|
Persistent review updated to latest commit fd98fc5 |
Docs-only, one file. Follow-up to the page merged as #1357.
Why. Trap 1's table had rows for
a5271d9and4a375bf2d, and main now pinsed43da0b4— on a page whose whole point is that the zero-adapter offset is a property of a pin. Measured on a build of that pin before writing the row: source tree ated43da0b4with an emptygit status(a second tree at the same sha on this box has four modified files — a lane's experiment, not the pin), frozen withsha256sumbefore use, device named in the log (HRX0on every factor,HRX0 LoRA buffer size = 24.06 MiB).a5271d9(c20)4a375bf2d(engine #1120)ed43da0b4(current)The finding is that the drift is transient, not monotonic.
4a375bf2dmoved the HRX base loss by +0.086 % and flipped the offset's sign;ed43da0b4puts all six numbers back toa5271d9's exact values. So a loss measured one pin ago is neither an upper nor a lower bound on today's — the page says to re-measure the row rather than interpolate between rows.Determinism, spelled out in the page as a two-row table rather than a claim: reference pair and this pin alternating, two runs each in one GPU ticket — base, zero-adapter, after-drop, ±ε, ×10 and Vulkan all equal on both pairs in all four runs, both PASS against
REF_HRX=2.265140.Two smaller corrections from the same round: the cost table named engine#973 but not the pin its numbers came from (now
32559003a, "the pin main carried then, not the current pin's"), and a new paragraph, "attribute the build before you record a row — the commit is not enough" — two trees at the same sha are not the same build, so a row records pin sha + working-tree cleanliness + the frozen libraries'sha256sum+ the device line.Review round 1 (VERDICT: READY, two count/precision notes, both taken): the reversion is no longer asserted —
a5271d9's ±ε and ×10 are on the page beside this pin's, in a table a reader can check; and "four identical fingerprints" now says six and matches the six columns.tools/copyright.py --checkrc 0.Refs #973, #1357.